Skip to content

Conversation

@winner245
Copy link
Contributor

@winner245 winner245 commented Dec 30, 2024

This PR simplifies __bitset::__init into a more compact and readable form, which avoids redundant computations of a size_t value and eliminates the overhead of a local array.

@winner245 winner245 force-pushed the simplify_bitset_init branch 5 times, most recently from 4446bf6 to d6bfafe Compare January 6, 2025 13:40
@winner245 winner245 marked this pull request as ready for review January 8, 2025 16:30
@winner245 winner245 requested a review from a team as a code owner January 8, 2025 16:30
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 8, 2025

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR simplifies __bitset::__init into a more compact and readable form, which avoids redundant computations of a size_t value and eliminates the overhead of a local array.


Full diff: https://github.com/llvm/llvm-project/pull/121357.diff

1 Files Affected:

  • (modified) libcxx/include/bitset (+7-16)
diff --git a/libcxx/include/bitset b/libcxx/include/bitset
index 919d2a0f07e096..57c29f5f85d27c 100644
--- a/libcxx/include/bitset
+++ b/libcxx/include/bitset
@@ -253,26 +253,16 @@ inline _LIBCPP_CONSTEXPR __bitset<_N_words, _Size>::__bitset() _NOEXCEPT
 
 template <size_t _N_words, size_t _Size>
 void __bitset<_N_words, _Size>::__init(unsigned long long __v, false_type) _NOEXCEPT {
-  __storage_type __t[sizeof(unsigned long long) / sizeof(__storage_type)];
-  size_t __sz = _Size;
-  for (size_t __i = 0; __i < sizeof(__t) / sizeof(__t[0]); ++__i, __v >>= __bits_per_word, __sz -= __bits_per_word)
-    if (__sz < __bits_per_word)
-      __t[__i] = static_cast<__storage_type>(__v) & (1ULL << __sz) - 1;
-    else
-      __t[__i] = static_cast<__storage_type>(__v);
-
-  std::copy(__t, __t + sizeof(__t) / sizeof(__t[0]), __first_);
-  std::fill(
-      __first_ + sizeof(__t) / sizeof(__t[0]), __first_ + sizeof(__first_) / sizeof(__first_[0]), __storage_type(0));
+  const size_t __ull_words = sizeof(unsigned long long) / sizeof(__storage_type);
+  for (size_t __i = 0; __i < __ull_words; ++__i, __v >>= __bits_per_word)
+    __first_[__i] = static_cast<__storage_type>(__v);
+  std::fill(__first_ + __ull_words, __first_ + _N_words, __storage_type(0));
 }
 
 template <size_t _N_words, size_t _Size>
 inline _LIBCPP_HIDE_FROM_ABI void __bitset<_N_words, _Size>::__init(unsigned long long __v, true_type) _NOEXCEPT {
   __first_[0] = __v;
-  if (_Size < __bits_per_word)
-    __first_[0] &= (1ULL << _Size) - 1;
-
-  std::fill(__first_ + 1, __first_ + sizeof(__first_) / sizeof(__first_[0]), __storage_type(0));
+  std::fill(__first_ + 1, __first_ + _N_words, __storage_type(0));
 }
 
 #  endif // _LIBCPP_CXX03_LANG
@@ -623,7 +613,8 @@ public:
 
   // 23.3.5.1 constructors:
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bitset() _NOEXCEPT {}
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bitset(unsigned long long __v) _NOEXCEPT : __base(__v) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bitset(unsigned long long __v) _NOEXCEPT
+      : __base(sizeof(unsigned long long) * CHAR_BIT <= _Size ? __v : __v & ((1ULL << _Size) - 1)) {}
   template <class _CharT, __enable_if_t<_IsCharLikeType<_CharT>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 explicit bitset(
       const _CharT* __str,

@winner245 winner245 force-pushed the simplify_bitset_init branch from d6bfafe to 0f02dfb Compare February 24, 2025 23:41
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of trying to simplify the C++03 code, we should look into removing __init entirely and replace the code with something like

bitset(unsigned long long __v) : vals() {
        vals[0] = static_cast<size_t>(__v);
  if (sizeof(size_t) == 4) {
    __first_[1] = _Size >= 2 * __bits_per_word
                   ? static_cast<size_t>(__v >> __bits_per_word)
                   : static_cast<size_t>((__v >> __bits_per_word) &
                          (__storage_type(1) << (_Size - __bits_per_word)) - 1);
  }
}

@winner245 winner245 force-pushed the simplify_bitset_init branch 3 times, most recently from e95d208 to cf0d4d7 Compare February 27, 2025 16:16
@winner245
Copy link
Contributor Author

winner245 commented Feb 27, 2025

After several attempts with a stage1 (generic-gcc, gcc-14, g++-14) CI failure, I realized that we still have to keep the initialization of the __first_ array in the constructor initializer list, because the constructor is constexpr since C++11, whereas C++11 constexpr constructor body does not allow statements other than the following: null statements, static_assert, typedef or alias declarations, using declarations/directives (https://en.cppreference.com/w/cpp/language/constexpr). Thus, gcc is unable to compile it (A reproducer) and that's why the generic-gcc CI fails.

Given this limitation, it seems that we still have to move the initialization back to the constructor initializer list. Since C++03 does not support {} in the constructor initializer list, I think we still need to keep the __init function for C++03. Please let me know if you have different opinions.

@ldionne
Copy link
Member

ldionne commented Mar 26, 2025

Gentle ping @philnik777 , there are questions for you here.

@winner245 winner245 force-pushed the simplify_bitset_init branch 3 times, most recently from a65e845 to 9bdcba2 Compare May 16, 2025 01:24
@philnik777
Copy link
Contributor

After several attempts with a stage1 (generic-gcc, gcc-14, g++-14) CI failure, I realized that we still have to keep the initialization of the __first_ array in the constructor initializer list, because the constructor is constexpr since C++11, whereas C++11 constexpr constructor body does not allow statements other than the following: null statements, static_assert, typedef or alias declarations, using declarations/directives (https://en.cppreference.com/w/cpp/language/constexpr). Thus, gcc is unable to compile it (A reproducer) and that's why the generic-gcc CI fails.

Given this limitation, it seems that we still have to move the initialization back to the constructor initializer list. Since C++03 does not support {} in the constructor initializer list, I think we still need to keep the __init function for C++03. Please let me know if you have different opinions.

I'd like to say "let's just drop GCC C++11 support", but I guess that's not trivial to get through. Could you file a bug against GCC and ask for them to implement the Clang extension? Otherwise LGTM.

@winner245 winner245 force-pushed the simplify_bitset_init branch from 9bdcba2 to d578748 Compare May 21, 2025 16:47
@ldionne
Copy link
Member

ldionne commented May 28, 2025

Shipping despite CI failures cause our CI is super unstable right now.

@ldionne ldionne merged commit 4608df5 into llvm:main May 28, 2025
604 of 656 checks passed
@winner245 winner245 deleted the simplify_bitset_init branch June 1, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants